PR #5312 Code Review: G1 Rough-Terrain Tuning (WIP) + FrameView + Newton Improvements

This PR contains three interconnected workstreams:

1. G1 rough-terrain locomotion tuning (WIP) with Newton-specific armature/damping presets
2. FrameView architecture refactoring to abstract backend-specific frame views
3. Newton backend improvements including shape margin fixes and MDP warp compatibility

Overall Assessment: REQUEST_CHANGES

While this PR contains valuable architectural improvements, there are several issues that need addressing before merge.

🔴 Critical Issues

1. preset() Function Signature Ambiguity

File: source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py

The preset() function uses **kwargs with a magic default key, but this creates ambiguity when users want a preset named "default":

def preset(default=_MISSING, **kwargs):
    if default is _MISSING and "default" not in kwargs:
        raise ValueError("preset() requires 'default=' or a kwarg named 'default'")

Issue: The function allows both preset(default=X) and preset(newton=Y, default=X) but the semantics differ - in the first case default is positional, in the second it's a kwarg. This is confusing.

Recommendation: Use explicit dict construction: preset({"default": X, "newton": Y}) or require all keys be kwargs.

2. ScalarPreset Serialization Asymmetry

File: source/isaaclab_tasks/isaaclab_tasks/utils/hydra.py:46-73

The ScalarPreset class implements to_dict() but from_dict() would reconstruct a plain scalar, not a ScalarPreset. This breaks round-trip serialization:

sp = preset(default=0.01, newton=0.03)
d = sp.to_dict()  # Returns resolved scalar
# Cannot reconstruct ScalarPreset from d

Recommendation: Either document this limitation clearly or implement proper serialization.

3. Missing Type Annotations in New Termination Functions

File: source/isaaclab/isaaclab/envs/mdp/terminations.py:161-185

New functions body_lin_vel_out_of_limit and body_ang_vel_out_of_limit are missing return type annotations and have inconsistent docstring format:

def body_lin_vel_out_of_limit(
    env: ManagerBasedRLEnv,
    max_velocity: float,
    asset_cfg: SceneEntityCfg = SceneEntityCfg("robot"),
) -> torch.Tensor:  # ✓ Has return type

Good - these are correct. But the docstrings lack Args/Returns sections that other functions in this file have.

🟡 Medium Issues

4. Hardcoded CHILD_OFFSET and ATOL Constants

File: source/isaaclab/isaaclab/sim/views/usd_frame_view.py:29-30

CHILD_OFFSET = (0.1, 0.0, 0.05)
ATOL = 1e-5

These magic constants lack documentation explaining their purpose:

• Why 0.1, 0.0, 0.05 specifically for child offset?
• Is ATOL=1e-5 appropriate for all use cases?

Recommendation: Add docstrings explaining these values' origins and when they might need adjustment.

5. RayCaster Major Refactoring Without Migration Path

File: source/isaaclab/isaaclab/sensors/ray_caster/ray_caster.py

The complete removal of _obtain_trackable_prim_view() (75+ lines) and switch to FrameView is a breaking change. The old code had specific handling for:

• ArticulationView detection
• RigidBodyView detection
• XformPrimView fallback with warnings

Issue: The new FrameView abstraction assumes the backend handles this, but there's no deprecation warning or migration guide.

Recommendation: Add a CHANGELOG entry and migration notes for users relying on the old behavior.

6. feet_slide Body ID Slicing Hack

File: source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/mdp/rewards.py:87-88

# Use sensor body count to slice asset velocities — ensures matching dimensions
# even when Newton reports duplicate body entries for closed-loop constraints.
body_vel = wp.to_torch(asset.data.body_lin_vel_w)[:, asset_cfg.body_ids[:contacts.shape[1]], :2]

Issue: This is a workaround for a Newton backend inconsistency, not a proper fix. The comment acknowledges Newton "reports duplicate body entries" - this should be fixed in Newton, not papered over in reward functions.

Recommendation: File a follow-up issue to fix Newton's body reporting. This workaround should be temporary.

7. G1 Action Space Comment Mismatch

File: source/isaaclab_tasks/isaaclab_tasks/manager_based/locomotion/velocity/config/g1/rough_env_cfg.py:35-36

# We need to include the joints above as well, to get the 37 DOF action space.
# This is less problematic since we can let the finger joints free-floating

The PR removes 14 finger joints from the action space (37 → 23 DOF), but the comment still references "37 DOF action space". This is stale documentation.

🟢 Minor Issues / Suggestions

8. Unused Import in interactive_scene.py

File: source/isaaclab/isaaclab/scene/interactive_scene.py

The PR removes several imports but the TYPE_CHECKING block imports DeformableObject and SurfaceGripper which may not be used if those backends aren't active.

9. Test Coverage for ScalarPreset

File: source/isaaclab_tasks/test/test_hydra.py

The test file is comprehensive (1200+ lines) but I don't see explicit tests for:

• ScalarPreset with all-same-value presets (edge case)
• preset() with invalid/empty kwargs
• Round-trip serialization (which would catch issue #2)

10. torch.norm → torch.linalg.norm Migration

Throughout rewards.py and terminations.py, the PR migrates from deprecated torch.norm to torch.linalg.norm. This is good practice, but should be noted in the PR description for searchability.

✅ Positive Observations

1. FrameView Architecture: The BaseFrameView → FrameView → backend-specific implementations pattern is clean and follows good OOP principles.

2. preset() Hydra Integration: The concept of backend-conditional values (preset(default=X, newton=Y)) is elegant for multi-backend configs.

3. Newton Shape Margin Fix: The CHANGELOG entry for contact_margin → margin typo fix is well-documented.

4. Test Infrastructure: The expanded test_hydra.py with 1200+ lines of tests shows good coverage of the new preset system.

5. Warp Compatibility: The systematic wp.to_torch() wrapping in MDP functions enables Newton backend support.

Recommended Actions

1. Clarify preset() function signature - consider explicit dict API
2. Document magic constants in usd_frame_view.py
3. Fix stale comment about 37 DOF in G1 config
4. Add migration notes for RayCaster changes
5. File follow-up issue for Newton duplicate body entries
6. Add missing test cases for ScalarPreset edge cases

Reviewed at commit: a67b001

WIP PR Review: G1 Rough-Terrain Tuning

Reviewed the WIP commit a67b0017 which adds G1-specific rough terrain tuning on top of the parent PR #5298.

Summary

This is a well-documented WIP PR that addresses a real training issue (14 finger joints inflating action_rate_l2 penalty 3× vs H1) and applies the established Newton armature/damping preset pattern from Go2. The measured results (+20.54 PhysX, +9.61 Newton) demonstrate meaningful improvement.

Overall

For a WIP PR focused on recording tuning progress, this is clean and well-structured. The code follows established patterns (preset() for Newton-specific values, actuators["legs"] matching the G1_CFG definition). The detailed PR description with measured results is appreciated.

When promoting from WIP:

• Consider whether the damping bump (5.0→20.0) is worth the base_contact regression noted in the PR description (7%→18.7%)
• The armature-only variant (+8.10 reward) may be a cleaner landing point than armature+damping (+9.61 with contact regression)
• As noted in the PR, this could potentially be split: fingers-out-of-action fix (clean PhysX improvement) could land independently of the Newton tuning work

No blocking issues found. The one inline comment is a minor documentation fix.

🤖 Isaac Lab Review Bot

Summary

This PR (Part 3 of rough locomotion) adds Newton backend support for the G1 biped on rough terrain. The implementation is minimal and well-documented: a single preset(default=3000, newton=5000) on max_iterations in G1RoughPPORunnerCfg is the only G1-specific change. The PR description provides thorough ablation data showing Newton reaches PhysX-equivalent quality (reward ≈ +16, ep_len ≈ 984) at iter 5000 vs PhysX's plateau at iter 3000.

This is a clean, well-scoped change. The decision to bump iteration budget rather than tune physics/reward terms keeps the env config engine-agnostic, following precedent from Allegro Hand (5000 iter) and Spot (20000 iter).

Architecture Impact

No cross-module impact from the G1-specific changes. The max_iterations preset is localized to the PPO runner config. The rest of the PR stacks parent changes from #5298 (biped Newton support) and #5248 (quadruped Newton support), which have broader impact but were covered in prior reviews.

Implementation Verdict

Ship it — The G1-specific change (commit 99f50d7d) is correct and follows established patterns. The comment explains the rationale thoroughly.

Test Coverage

Acceptable for this change type. This is a hyperparameter tuning PR (iteration count), not a code behavior change. The existing rough-terrain training infrastructure provides implicit coverage. The PR description includes ablation data demonstrating the change achieves parity:

The data validates the 5000 iteration preset is sufficient for Newton to match PhysX plateau quality.

CI Status

Only labeler check is present (passed ✅). No pre-commit/lint/test failures detected.

Branch Status

⚠️ This branch has diverged from develop (30 commits behind, 29 ahead). Consider rebasing before merge to pick up recent changes and ensure CI runs against current develop.

Findings

No new issues found in the G1-specific changes (commit 99f50d7d). The change is minimal, well-documented, and follows established patterns.

Prior review comments still applicable: The stacked commits from parent PRs have existing review comments (FrameView constants documentation, Newton body reporting workaround, etc.) that should be addressed in those parent PRs.

Reviewed at commit: 99f50d7